Codecov Report

Patch coverage: 63.63% and project coverage change: -0.14 ⚠️

Comparison is base (81007e1) 87.50% compared to head (58912bf) 87.36%.

@@            Coverage Diff             @@
##           master     #239      +/-   ##
==========================================
- Coverage   87.50%   87.36%   -0.14%     
==========================================
  Files          18       18              
  Lines        2281     2288       +7     
==========================================
+ Hits         1996     1999       +3     
- Misses        285      289       +4     

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

can we have benchmark? this is hot spot in our code

can we have benchmark? this is hot spot in our code

It looks still a little bit messy, I just pushed to let you know I'm on that. I try to clean/optimize this later a little bit and then we can do some benchmarks 👍

@Moelf what benchmarks are you having in mind ... when doing:

using UnROOT
using BenchmarkTools

f = ROOTFile("test.root")
t = LazyTree(f, "gen")
@btime $t[1:1000000]

I get for my testfile:

• master: 46.633 ms (479 allocations: 60.00 MiB)
• fix-recovered-basket-indexrange: 45.901 ms (479 allocations: 60.00 MiB)

So not really a change for the case of no recovered TBasket.

maybe hit it with

for evt in tree
    evt.branch
end

tree[range] may have short cut

can we have an even more basic test, that is not doing branch[i:j]?

why can't we just have res = [evt.float_branch for evt in tree]?

also why are we adding tests for tree_with_large_array.root? does this file have recovered basket all along?

I think there is no sample file with a recovered tbasket which can be used that easy at the moment. So the idea was to add at least some tests for the getindex method for UnitRange for the standard use case. In order to make sure the added code doesn't break whats already there. @tamasgal suggested to sum up bunches inside the branch. As the sample files should be not replaced at some point, I thought some hardcoded values would be a nice test, because with that I don't rely on a other function, e.g. by creating the test values by accessing each index one by one.

I see. According to code coverage, we're not testing any of the added logic anyway, which is clearly gonna be the case until we have a sample file, we can wait I guess

Yes, it's difficult to find a small file with recovered baskets. I was not able to produce one yet.

I think it's fine like this. @8me verified that the indexing works with one of our large files and I'd say it's good enough for now. The -1 propagation is not the nicest approach but I want to work on the basket parsing mechanism anyways and will probably refactor that completely, now that I have more experience with all these structures.

We can go ahead with the merge, just need one or two lines of comments regarding -1.

Thanks for rebasing @8me. I just checked manually the type-safety (see #242) and everything seems fine.

Let's merge...